Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] Support usage of separate Linode API for the domain client #436

Merged
merged 13 commits into from
Aug 5, 2024

Conversation

mabojars
Copy link
Contributor

@mabojars mabojars commented Aug 2, 2024

What this PR does / why we need it:

  • Currently, CAPL does not allow targeting different Linode API instances for the domain client, and the client for the remaining resources. This use case is required by an internal project.
  • Also, using custom CA certs is broken, due to the way CAPL initializes the linodego client (see below).

Which issue(s) this PR fixes: N/A

Special notes for your reviewer:

  • Exposes new environment variables DNS Linode client configuration:
    • LINODE_DNS_URL - equivalent of the LINODE_URL, but applies only to the DNS client.
    • LINODE_DNS_CA - equivalent of the LINODE_CA, but applies only to the DNS client. If we target a different API instance than LINODE_URL, we might also need to provide different root CA certs.
    • If the above are not provided, default linodego logic applies (same as before).
  • Implements a struct for Linode client configuration (ClientConfig), to keep things from becoming messy with the new parameters. Used as a DTO right after startup.
  • Adds an info level log informing about the fallback of LINODE_DNS_TOKEN to LINODE_TOKEN happening. I've noticed this happens silently, and it would be nice to know.
  • Drops the usage of the oauth2.Transport. This caused the custom TLS configuration to be silently discarded, due to an unexpected interaction with resty. The flow would go like this:
  • Removes emitted log line counting in for controller test suite with events/logs. We've agreed with @eljohnson92 this does not bring much value, while being troublesome to maintain and debug.

TODOs:

  • squashed commits - I'd prefer to squash on merge. The separate commits add valuable context.
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@mabojars mabojars force-pushed the fix-dns-client-customization branch from c154348 to b17e444 Compare August 2, 2024 09:32
@mabojars mabojars changed the title [feat] Support independent Linode domain client configuration [feat] Support usage of separate Linode API for the domain client Aug 2, 2024
Copy link

codecov bot commented Aug 2, 2024

Codecov Report

Attention: Patch coverage is 50.56180% with 44 lines in your changes missing coverage. Please review.

Project coverage is 66.14%. Comparing base (1b4a2b9) to head (b835501).

Files Patch % Lines
cmd/main.go 0.00% 33 Missing ⚠️
cloud/scope/common.go 76.00% 3 Missing and 3 partials ⚠️
controller/linodemachine_controller.go 0.00% 2 Missing ⚠️
controller/linodecluster_controller.go 0.00% 1 Missing ⚠️
controller/linodeplacementgroup_controller.go 0.00% 1 Missing ⚠️
controller/linodevpc_controller.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #436      +/-   ##
==========================================
- Coverage   66.27%   66.14%   -0.13%     
==========================================
  Files          76       76              
  Lines        3887     3908      +21     
==========================================
+ Hits         2576     2585       +9     
- Misses       1122     1131       +9     
- Partials      189      192       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mabojars mabojars force-pushed the fix-dns-client-customization branch from d34d064 to 3e8d316 Compare August 2, 2024 09:55
@mabojars mabojars force-pushed the fix-dns-client-customization branch from 096434f to 2d35126 Compare August 2, 2024 13:33
@mabojars mabojars marked this pull request as ready for review August 2, 2024 14:08
@mabojars mabojars force-pushed the fix-dns-client-customization branch from 2d35126 to 6cd8baa Compare August 2, 2024 14:11
eljohnson92
eljohnson92 previously approved these changes Aug 2, 2024
Copy link
Contributor

@AshleyDumaine AshleyDumaine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mabojars mabojars merged commit 1590966 into main Aug 5, 2024
11 of 13 checks passed
@mabojars mabojars deleted the fix-dns-client-customization branch August 5, 2024 07:56
okokes-akamai added a commit to okokes-akamai/linode-cloud-controller-manager that referenced this pull request Aug 15, 2024
We're hitting the same issue as here: linode/cluster-api-provider-linode#436

We use a custom CA and it doesn't get used.

Given that we construct an http.Transport that is of a different
concrete type than http.Transport (it just needs to be a RoundTripper),
resty fails and defaults to an empty client, discarding our CA.
okokes-akamai added a commit to linode/linode-cloud-controller-manager that referenced this pull request Aug 15, 2024
We're hitting the same issue as here: linode/cluster-api-provider-linode#436

We use a custom CA and it doesn't get used.

Given that we construct an http.Transport that is of a different
concrete type than http.Transport (it just needs to be a RoundTripper),
resty fails and defaults to an empty client, discarding our CA.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants